-
Notifications
You must be signed in to change notification settings - Fork 601
document t/test.pl #23700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
document t/test.pl #23700
Conversation
|
This looks very good but I'll have to defer a thorough look until tomorrow. |
jkeenan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot to review here. The sections up through Test::More differences look good to me. I suggest getting different people to review the various remaining sections:
204:=head1 Extended test functions
438:=head1 Utility functions
661:=head1 Runperl Arguments
736:=head1 run_multiple_progs() tests
t/test_pl.pod
Outdated
|
|
||
| =head1 Extended test functions | ||
|
|
||
| These test some condition and produce TAP for a test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point in my life, I know what TAP is when I see it ... but I had to stop and think what the acronym stands for. Perhaps a link to someplace that defines it would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly going for "produces a counted test" but I'm not sure that's better
t/test_pl.pod
Outdated
|
|
||
| =back | ||
|
|
||
| There's also some variables which detect useful configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar: should be "There are also ..."
|
I think we should just merge this. It lgtm, but there is a lot of stuff here that I didn't know about. If something comes up where this isn't quite right, what harm will have been done. We would just fix it. End-users would not be affected. |
I got sick of reverse engineering run_multiple_progs() tests and runperl() arguments each time I needed to use them. There's some documentation in comments inline but it's pretty variable and less accessible than pod. Since (in theory anyway) we want test.pl to exercise as little of perl as possible, the POD doesn't belong in test.pl itself. So I've put this in t/test_pl.pod since it's not really end user documentation that would belong in pod/.
podcheck normally excludes t/ but I do want t/test_pl.pod checked. Updating is_pod_file() to allow only this file seems more complex than just adding t/test_pl.pod afterwards, so just add it, just as t/porting/podcheck.t is added.
ebfdfc0 to
d8cf780
Compare
|
added it to podcheck and fixed some issues ( |
|
still lgtm |
I got sick of reverse engineering run_multiple_progs() tests and runperl() arguments each time I needed to use them.
There's some documentation in comments inline but it's pretty variable and less accessible than pod.
Since (in theory anyway) we want test.pl to exercise as little of perl as possible, the POD doesn't belong in test.pl itself.
So I've put this in t/test_pl.pod since it's not really end user documentation that would belong in pod/.
Suggestions welcome for a better location.